Skip to content

[libc++] Optimize __hash_table::erase(iterator, iterator) #152471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[libc++] Optimize __hash_table::erase(iterator, iterator) #152471

wants to merge 1 commit into from
+49 −5

Conversation

philnik777
Copy link
Contributor 

----------------------------------------------------------------------------------------------------------------
Benchmark                                                                                        old         new
----------------------------------------------------------------------------------------------------------------
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/0          450 ns      446 ns
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/32        1017 ns      614 ns
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/1024     16035 ns     7747 ns
std::unordered_map<int, int>::erase(iterator, iterator) (erase half the container)/8192    122107 ns    73020 ns

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 7, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command: 
git-clang-format --diff HEAD~1 HEAD --extensions  -- libcxx/include/__hash_table
View the diff from clang-format here. 
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index 2f0f9457f..872fbd480 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -1854,7 +1854,7 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __first, const_it
   // current node
   __next_pointer __current = __first.__node_;
   size_type __bucket_count = bucket_count();
-  size_t __chash = std::__constrain_hash(__current->__hash(), __bucket_count);
+  size_t __chash           = std::__constrain_hash(__current->__hash(), __bucket_count);
   // find previous node
   __next_pointer __before_first = __bucket_list_[__chash];
   for (; __before_first->__next_ != __current; __before_first = __before_first->__next_)

@philnik777 philnik777 marked this pull request as ready for review August 11, 2025 19:53
@philnik777 philnik777 requested a review from a team as a code owner August 11, 2025 19:53
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 11, 2025
@llvmbot
Copy link
Member

llvmbot commented Aug 11, 2025

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes 
----------------------------------------------------------------------------------------------------------------
Benchmark                                                                                        old         new
----------------------------------------------------------------------------------------------------------------
std::unordered_map&lt;int, int&gt;::erase(iterator, iterator) (erase half the container)/0          450 ns      446 ns
std::unordered_map&lt;int, int&gt;::erase(iterator, iterator) (erase half the container)/32        1017 ns      614 ns
std::unordered_map&lt;int, int&gt;::erase(iterator, iterator) (erase half the container)/1024     16035 ns     7747 ns
std::unordered_map&lt;int, int&gt;::erase(iterator, iterator) (erase half the container)/8192    122107 ns    73020 ns

Full diff: https://github.com/llvm/llvm-project/pull/152471.diff

1 Files Affected:

  • (modified) libcxx/include/__hash_table (+49-5)
diff --git a/libcxx/include/__hash_table b/libcxx/include/__hash_table
index dacc152030e14..2f0f9457f1416 100644
--- a/libcxx/include/__hash_table
+++ b/libcxx/include/__hash_table
@@ -1848,12 +1848,56 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __p) {
 template <class _Tp, class _Hash, class _Equal, class _Alloc>
 typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
 __hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __first, const_iterator __last) {
-  for (const_iterator __p = __first; __first != __last; __p = __first) {
-    ++__first;
-    erase(__p);
+  if (__first == __last)
+    return iterator(__last.__node_);
+
+  // current node
+  __next_pointer __current = __first.__node_;
+  size_type __bucket_count = bucket_count();
+  size_t __chash = std::__constrain_hash(__current->__hash(), __bucket_count);
+  // find previous node
+  __next_pointer __before_first = __bucket_list_[__chash];
+  for (; __before_first->__next_ != __current; __before_first = __before_first->__next_)
+    ;
+
+  __next_pointer __end = __last.__node_;
+
+  // If __before_first is in the same bucket, clear this bucket first without re-linking it
+  if (__before_first != __first_node_.__ptr() &&
+      std::__constrain_hash(__before_first->__hash(), __bucket_count) == __chash) {
+    while (__current != __end) {
+      if (auto __next_chash = std::__constrain_hash(__current->__hash(), __bucket_count); __next_chash != __chash) {
+        __chash = __next_chash;
+        break;
+      }
+      auto __next = __current->__next_;
+      __node_traits::deallocate(__node_alloc(), __current->__upcast(), 1);
+      __current = __next;
+      --__size_;
+    }
   }
-  __next_pointer __np = __last.__node_;
-  return iterator(__np);
+
+  while (__current != __end) {
+    auto __next = __current->__next_;
+    __node_traits::deallocate(__node_alloc(), __current->__upcast(), 1);
+    __current = __next;
+    --__size_;
+
+    // When switching buckets, set the old bucket to be empty and update the next bucket to have __before_first as its
+    // before-first element
+    if (__next) {
+      if (auto __next_chash = std::__constrain_hash(__next->__hash(), __bucket_count); __next_chash != __chash) {
+        __bucket_list_[__chash] = nullptr;
+        __chash                 = __next_chash;
+        __bucket_list_[__chash] = __before_first;
+      }
+    }
+  }
+
+  // re-link __before_start with __last
+  __before_first->__next_ = __current;
+
+  return iterator(__last.__node_);
 }
 
 template <class _Tp, class _Hash, class _Equal, class _Alloc>

ldionne
ldionne reviewed Aug 13, 2025
View reviewed changes
libcxx/include/__hash_table
@@ -1848,12 +1848,56 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __p) {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also show the results for the other unordered containers, including the multi ones.

libcxx/include/__hash_table
@@ -1848,12 +1848,56 @@ __hash_table<_Tp, _Hash, _Equal, _Alloc>::erase(const_iterator __p) {
template <class _Tp, class _Hash, class _Equal, class _Alloc>
typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::iterator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a release note.

libcxx/include/__hash_table
for (const_iterator __p = __first; __first != __last; __p = __first) {
++__first;
erase(__p);
if (__first == __last)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a summary of what this optimization is in the commit message.

libcxx/include/__hash_table
for (; __before_first->__next_ != __current; __before_first = __before_first->__next_)
;

__next_pointer __end = __last.__node_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
__next_pointer __end = __last.__node_;
__next_pointer __last_node = __last.__node_;

end is a bit confusing since it's not the end of the container.

libcxx/include/__hash_table
break;
}
auto __next = __current->__next_;
__node_traits::deallocate(__node_alloc(), __current->__upcast(), 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to destroy the node here? You seem to only deallocate. If that's indeed a bug, I think we need a test that finds this issue.

libcxx/include/__hash_table

__next_pointer __end = __last.__node_;

// If __before_first is in the same bucket, clear this bucket first without re-linking it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// If __before_first is in the same bucket, clear this bucket first without re-linking it
// If __before_first is in the same bucket (i.e. the first element we erase is not the first in its bucket), clear this bucket first without re-linking it

libcxx/include/__hash_table
}
}

// re-link __before_start with __last
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// re-link __before_start with __last
// re-link __before_first with __last

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldionne ldionne ldionne left review comments

Assignees

@philnik777 philnik777

Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philnik777 @llvmbot @ldionne